Automated Test: blocked-email-validation-post #318
Conversation
… many times each email address is blocked, and last time it was blocked. Move email validation out of User model and into EmailValidator. Signup form remembers which email addresses have failed and shows validation error on email field.
📝 WalkthroughWalkthroughThis PR introduces an email blocking system for account registration. It adds a Changes
Sequence DiagramsequenceDiagram
participant User as User (Frontend)
participant Controller as CreateAccount<br/>Controller
participant UsersAPI as UsersController<br/>#create
participant Validator as EmailValidator
participant BlockedEmail as BlockedEmail
participant DB as Database
User->>Controller: Submit account creation form
Controller->>Controller: Check emailValidation computed property
Controller->>BlockedEmail: Check rejectedEmails array
alt Email in rejectedEmails
Controller-->>User: Validation error (email invalid)
else Email not rejected yet
Controller->>UsersAPI: POST /users (accountEmail)
UsersAPI->>Validator: Validate email attribute
Validator->>BlockedEmail: should_block?(email)
BlockedEmail->>DB: Find by email
DB-->>BlockedEmail: BlockedEmail record (if exists)
alt Record exists and action_type is block
BlockedEmail->>DB: Increment match_count, update last_match_at
BlockedEmail-->>Validator: true (should block)
Validator->>UsersAPI: Add blocked error
else No record or action_type is do_nothing
BlockedEmail-->>Validator: false (don't block)
Validator-->>UsersAPI: Validation passes
end
alt Validation failed (email blocked)
UsersAPI-->>Controller: Error response with errors hash and values
Controller->>Controller: Push email to rejectedEmails (via pushObject)
Controller-->>User: Display error, email now in rejected list
else Validation succeeded
UsersAPI-->>Controller: Success response
Controller-->>User: Account created
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/models/blocked_email.rb`:
- Around line 11-17: The should_block? method is doing a read/modify/write which
loses increments under concurrency and triggers validations/callbacks; replace
the record fetch + record.match_count += 1 + record.save with a single atomic DB
update such as BlockedEmail.where(email: email).update_all(["match_count =
COALESCE(match_count,0) + 1, last_match_at = ?", Time.zone.now]) (or use
ActiveRecord::Base.increment_counter if you only need to increment) so the
update occurs atomically and bypasses validations/callbacks while still updating
match_count and last_match_at.
In `@app/models/user.rb`:
- Around line 46-47: Add case-insensitive email normalization to the User model:
implement a before_validation callback (e.g., before_validation
:normalize_email) that downcases and strips the email attribute, and update the
uniqueness validation on :email to be case_sensitive: false so validations use
the normalized value; reference the User model validations and the
normalize_email callback/method when making the change.
In `@lib/validators/email_validator.rb`:
- Around line 18-21: The current email_in_restriction_setting? method builds a
loose regex that allows substrings (e.g., user@example.com.evil); to fix it,
split the pipe-separated setting into individual domains, escape each domain
with Regexp.escape, join them with | into a non-capturing group, and build a
anchored, case-insensitive regex that requires the domain to be exactly at the
end of the address (for example: /\A[^@]+@(?:domain1|domain2)\z/i); use this
regex in email_in_restriction_setting? (and prefer match? or regexp.match
instead of relying on =~ return value) so only exact domain matches pass.
🧹 Nitpick comments (3)
spec/components/validators/email_validator_spec.rb (2)
3-21: Consider adding test coverage for domain whitelist/blacklist validation.The AI summary indicates
EmailValidatoralso enforces domain restrictions viaSiteSetting(whitelist/blacklist). The current tests only cover theBlockedEmail.should_block?scenario. Consider adding tests for:
- Email domain blacklist matching
- Email domain whitelist enforcement
4-4: Minor: Extra empty line at block body beginning.Static analysis flagged an extra empty line. This is a minor formatting nit.
🧹 Proposed fix
describe EmailValidator do - let(:record) { Fabricate.build(:user, email: "bad@spamclub.com") }spec/models/blocked_email_spec.rb (1)
9-9: Prefereqmatcher over==for clearer failure messages.Using
should eq(BlockedEmail.actions[:block])provides better error output thanshould ==.🧹 Proposed fix
- BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block] + BlockedEmail.create(email: email).action_type.should eq(BlockedEmail.actions[:block])
| def self.should_block?(email) | ||
| record = BlockedEmail.where(email: email).first | ||
| if record | ||
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check where match_count is used to understand the impact
rg -n "match_count" --glob '*.rb'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 374
🏁 Script executed:
# Look at the full BlockedEmail model to understand context and any existing concurrency controls
find . -name "blocked_email.rb" -type f | head -5Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 117
🏁 Script executed:
# Read the full BlockedEmail model
cat -n app/models/blocked_email.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 805
🏁 Script executed:
# Read the spec to understand expected behavior
cat -n spec/models/blocked_email_spec.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1987
🏁 Script executed:
# Check if there are any transaction blocks or locking mechanisms in the Rails app
rg -n "transaction|with_lock" --glob '*.rb' app/models/ | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1299
🏁 Script executed:
# Check all callers of should_block? to ensure the refactor won't break anything
rg -n "should_block\?" --glob '*.rb'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 676
🏁 Script executed:
# Verify the suggested update_all syntax is complete
rg -n "update_all" --glob '*.rb' app/models/ | head -10Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1354
Use atomic update to prevent lost match_count increments under concurrent access.
The read/modify/write cycle loses increments when multiple requests fetch the same record concurrently. Additionally, each match unnecessarily triggers validations and callbacks.
Suggested fix
- if record
- record.match_count += 1
- record.last_match_at = Time.zone.now
- record.save
- end
+ if record
+ BlockedEmail.where(id: record.id)
+ .update_all(["match_count = match_count + 1, last_match_at = ?", Time.zone.now])
+ end🤖 Prompt for AI Agents
In `@app/models/blocked_email.rb` around lines 11 - 17, The should_block? method
is doing a read/modify/write which loses increments under concurrency and
triggers validations/callbacks; replace the record fetch + record.match_count +=
1 + record.save with a single atomic DB update such as BlockedEmail.where(email:
email).update_all(["match_count = COALESCE(match_count,0) + 1, last_match_at =
?", Time.zone.now]) (or use ActiveRecord::Base.increment_counter if you only
need to increment) so the update occurs atomically and bypasses
validations/callbacks while still updating match_count and last_match_at.
| validates :email, presence: true, uniqueness: true | ||
| validates :email, email: true, if: :email_changed? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for email normalization patterns in the codebase
echo "=== Searching for email normalization patterns ==="
rg -n "normalize_email|Email\.downcase|downcase.*email|email\.downcase" --glob '*.rb' -A 2
echo -e "\n=== Searching for before_validation hooks related to email ==="
rg -n "before_validation.*email|before_save.*email" --glob '*.rb' -A 2
echo -e "\n=== Checking User model content (app/models/user.rb) ==="
if [ -f "app/models/user.rb" ]; then
wc -l app/models/user.rb
cat -n app/models/user.rb
else
echo "File not found at app/models/user.rb"
fi
echo -e "\n=== Searching for case-insensitive email indexes in migrations ==="
rg -n "case_sensitive|:unique.*email|index.*email" --glob 'db/migrate/*.rb' -A 2
echo -e "\n=== Checking for schema.rb ==="
if [ -f "db/schema.rb" ]; then
rg -n "email" db/schema.rb -B 2 -A 2
else
echo "schema.rb not found"
fiRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 29192
Add email normalization to prevent case-sensitive duplicates.
The uniqueness: true validation is case-sensitive by default. Without email normalization in the User model, mixed-case duplicates like "User@Example.com" and "user@example.com" can be stored, breaking email lookups that rely on case-insensitive comparison. The Invite model already normalizes email in a before_validation hook; apply the same pattern here.
+ before_validation :normalize_email
validates :email, presence: true, uniqueness: { case_sensitive: false }
validates :email, email: true, if: :email_changed?
+
+ def normalize_email
+ self.email = Email.downcase(email) if email.present?
+ end🤖 Prompt for AI Agents
In `@app/models/user.rb` around lines 46 - 47, Add case-insensitive email
normalization to the User model: implement a before_validation callback (e.g.,
before_validation :normalize_email) that downcases and strips the email
attribute, and update the uniqueness validation on :email to be case_sensitive:
false so validations use the normalized value; reference the User model
validations and the normalize_email callback/method when making the change.
| def email_in_restriction_setting?(setting, value) | ||
| domains = setting.gsub('.', '\.') | ||
| regexp = Regexp.new("@(#{domains})", true) | ||
| value =~ regexp |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual file structure and locate email_validator.rb
find . -name "email_validator.rb" -type fRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 123
🏁 Script executed:
# Read the email_validator.rb file to verify the code at lines 18-21
if [ -f "lib/validators/email_validator.rb" ]; then
cat -n "lib/validators/email_validator.rb" | head -30
fiRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1095
🏁 Script executed:
# Search for tests of email_in_restriction_setting? method
rg -n "email_in_restriction_setting" --glob "*.rb"Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 362
🏁 Script executed:
# Search for documentation or usage of email domain settings
rg -n "email.*domain.*setting\|setting.*email.*domain" --glob "*.rb" --glob "*.yml" -A 3Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Search for SiteSetting definitions or tests related to email domain settings
rg -n "email_domains_(whitelist|blacklist)" --glob "*.rb" -B 2 -A 2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 6573
🏁 Script executed:
# Search for any test files that might test this validator
find . -path "*/test*" -o -path "*/spec*" | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1620
🏁 Script executed:
# Look for tests of EmailValidator
rg -n "EmailValidator\|email_in_restriction" --glob "*spec*.rb" --glob "*test*.rb" -B 2 -A 5Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
Add anchors to prevent subdomain bypasses in email domain matching.
Lines 18-21 uses substring regex matching without anchors, allowing user@example.com.evil to match example.com. The pattern @(example\.com) finds the substring anywhere in the email. Use \A and \z anchors and properly split pipe-separated domains to ensure exact domain matching only.
Suggested fix
- def email_in_restriction_setting?(setting, value)
- domains = setting.gsub('.', '\.')
- regexp = Regexp.new("@(#{domains})", true)
- value =~ regexp
- end
+ def email_in_restriction_setting?(setting, value)
+ return false if value.blank?
+ domains = setting.split(/[\s|,]+/).map { |d| Regexp.escape(d) }.reject(&:blank?)
+ regexp = /\A[^@]+@(?:#{domains.join('|')})\z/i
+ value =~ regexp
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def email_in_restriction_setting?(setting, value) | |
| domains = setting.gsub('.', '\.') | |
| regexp = Regexp.new("@(#{domains})", true) | |
| value =~ regexp | |
| def email_in_restriction_setting?(setting, value) | |
| return false if value.blank? | |
| domains = setting.split(/[\s|,]+/).map { |d| Regexp.escape(d) }.reject(&:blank?) | |
| regexp = /\A[^@]+@(?:#{domains.join('|')})\z/i | |
| value =~ regexp | |
| end |
🤖 Prompt for AI Agents
In `@lib/validators/email_validator.rb` around lines 18 - 21, The current
email_in_restriction_setting? method builds a loose regex that allows substrings
(e.g., user@example.com.evil); to fix it, split the pipe-separated setting into
individual domains, escape each domain with Regexp.escape, join them with | into
a non-capturing group, and build a anchored, case-insensitive regex that
requires the domain to be exactly at the end of the address (for example:
/\A[^@]+@(?:domain1|domain2)\z/i); use this regex in
email_in_restriction_setting? (and prefer match? or regexp.match instead of
relying on =~ return value) so only exact domain matches pass.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes